ci: reduce system tests execution time, split AppSec ubuntu matrix, rework All Green report uploads - #9197
ci: reduce system tests execution time, split AppSec ubuntu matrix, rework All Green report uploads#9197rochdev wants to merge 37 commits into
Conversation
Overall package sizeSelf size: 8.03 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.3 | 125.43 kB | 441.68 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: c74af92 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-08-13 00:32:48 Comparing candidate commit c74af92 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2319 metrics, 39 unstable metrics.
|
0f4fc4f to
239c810
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dd8d9a919
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
More details
The per-run concurrent upload architecture in all-green.mjs and the rewritten group-coverage.mjs are correct: processedRunIds prevents double-processing, downloadArtifacts is safe for concurrent calls (paths isolated by run ID), mergeLcov correctly concatenates lcov records including duplicate source files (Codecov sums server-side), and runUpload always resolves (never rejects) while reliably setting process.exitCode on non-zero exits. All 8 official tests and all 10 adversarial scenarios pass.
📊 Validated against 18 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 6dd8d9a · What is Autotest? · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b994d9fdd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
The new spawnUpload function in scripts/run-upload.mjs is missing a child.on('error', ...) listener. Node.js treats an unhandled error event on a ChildProcess as an uncaught exception, crashing all-green.mjs immediately instead of returning a failed UploadResult. The old shell-script approach (bash exit 127) handled this gracefully; the new spawn-based approach does not. Fix applied: add child.on('error', err => { output += err.message }) and use code ?? 1 in the close handler to cover the null code that spawn failures emit.
📊 Validated against 17 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 0b994d9 · What is Autotest? · Any feedback? Reach out in #autotest
6c4bedc to
79166f3
Compare
79166f3 to
7a3eabe
Compare
Each matrix cell in a sibling workflow (Node.js version, plugin partition) writes its own complete lcov report, so a shared source file gets an `SF:` block from every cell. Concatenating those blocks, as group-coverage.mjs did, produced a report with duplicate `SF:` sections per file; Codecov keeps only the last block for a file rather than summing across duplicates, which silently discarded almost all branch/function coverage once per-cell uploads were merged into one per-workflow upload (PR #9197's branch dropped from 10802 to 65 branches as a result). mergeLcov now sums DA:/FNDA:/BRDA: hit counts per file across cells instead, the way `lcov --add-tracefile` does. Generated by Claude Code.
Each matrix cell in a sibling workflow (Node.js version, plugin partition) writes its own complete lcov report, so a shared source file gets an `SF:` block from every cell. Concatenating those blocks, as group-coverage.mjs did, produced a report with duplicate `SF:` sections per file; Codecov keeps only the last block for a file rather than summing across duplicates, which silently discarded almost all branch/function coverage once per-cell uploads were merged into one per-workflow upload (PR #9197's branch dropped from 10802 to 65 branches as a result). mergeLcov now sums DA:/FNDA:/BRDA: hit counts per file across cells instead, the way `lcov --add-tracefile` does. Generated by Claude Code.
ca32fde to
3152da2
Compare
There was a problem hiding this comment.
The per-call limiter does not bound the aggregate burst when several workflows settle together, recreating the API pressure this change intends to remove. The custom lcov merger also silently collapses valid same-named functions from different source lines.
📊 Validated against 8 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 926d33c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
…med lcov function merge Addresses two Codex/Datadog Autotest review findings on PR #9197: - The per-`downloadArtifacts`-call worker pool let each concurrently processed sibling workflow open its own 10-download burst, recreating the aggregate GitHub API pressure the cap was meant to prevent. A module-scoped Semaphore now bounds every in-flight call together. - `mergeLcovRecord` keyed functions by name only, so two functions sharing a name at different lines (e.g. two closures both named `shared`) collapsed into one, undercounting FNF/FNH. FN/FNDA are now paired positionally and keyed by `line,name`. Generated by Claude Code.
The ubuntu job ran all four Node versions sequentially in a single job. Splitting into a matrix lets them run concurrently, cutting wall-clock time.
group-coverage.mjs bucketed cell reports per integration (~101 groups), each needing its own datadog-ci/codecov CLI invocation. .codecov.yml only gates the separate master-coverage flag, so per-integration flags carried no gating weight of their own - they just fed a coverage-by-plugin UI breakdown. Grouping by top-level area instead (appsec, apm-integrations, etc.) cuts that to ~12 groups, and bumping xargs -P from 10 to 20 lets all of them upload in a single concurrent batch instead of two sequential ones.
…nd fail loudly do-upload requires create-commit/create-report to already exist; without them it silently failed (exit 0, no report ever created) since --fail-on-error wasn't set. Add --fail-on-error to every codecovcli call and retry each with backoff so a transient failure doesn't immediately fail the whole All Green job. Generated by Claude Code
Downloading artifacts and merging coverage each logged their own line, so a single workflow run's report processing produced 4-5 scattered entries. Silence those intermediate steps and report their outcome (artifact count) in the same one-line summary the uploads already produce. Generated by Claude Code
sendCodecovNotifications fired unconditionally once all sibling workflows reached a final state, regardless of outcome. A failing suite's coverage run is expected to be lower than a healthy one, so notifying Codecov on a red commit reports a misleadingly low status against what should otherwise be a normal, unaffected coverage trend. Generated by Claude Code
Handle the 'error' event on spawned upload processes so an ENOENT doesn't crash All Green via an uncaught exception, and skip the Codecov notification when no run ever registered a commit/report (e.g. Dependabot PRs, whose coverage artifacts are skipped). Generated by Claude Code
Each matrix cell in a sibling workflow (Node.js version, plugin partition) writes its own complete lcov report, so a shared source file gets an `SF:` block from every cell. Concatenating those blocks, as group-coverage.mjs did, produced a report with duplicate `SF:` sections per file; Codecov keeps only the last block for a file rather than summing across duplicates, which silently discarded almost all branch/function coverage once per-cell uploads were merged into one per-workflow upload (PR #9197's branch dropped from 10802 to 65 branches as a result). mergeLcov now sums DA:/FNDA:/BRDA: hit counts per file across cells instead, the way `lcov --add-tracefile` does. Generated by Claude Code.
…n flag Every sibling workflow's merged coverage upload shared the same `coverage` flag, so Codecov's per-flag breakdown couldn't distinguish one workflow's coverage from another's. flagOf derives a Codecov-safe flag from the workflow's name (e.g. `test-optimization`, `appsec`) so each of the ~15 per-run uploads gets its own flag at no extra upload cost. Generated by Claude Code.
Dropping the JSON report in ffe3919 (lcov only) removed Codecov's branch-coverage total from ~10802 to near zero: Codecov's own cross-session merge overwrites rather than sums a shared file's lcov BRDA records when more than one sibling workflow's upload reports it, so a later zero-branch session (e.g. AppSec, whose branch coverage is broken by a separate pre-existing bug) clobbers an earlier session's real branch data. Restoring the merged istanbul JSON report — which Codecov reads branch/function coverage from and reconciles more reliably across sessions — for Codecov's upload only. Datadog keeps reading lcov, unchanged. Generated by Claude Code.
Generated by Claude Code.
…them A sibling workflow's upload bundles every matrix cell's junit XML into one datadog-ci call, which only tags the whole batch with one CI job name/ID - there was no way to tell which Node version a given test result came from once uploaded. mocha-junit-reporter now stamps each testsuite with a node_version property, and the upload lifts it into a real test.node_version tag via datadog-ci's documented property xpath pattern. Generated by Claude Code.
Each sibling workflow's junit upload previously let datadog-ci's --auto-discovery upload every matrix cell's XML file separately, one HTTP request per file. Merge every cell's report into a single XML document per run before uploading instead, now that each testcase carries a node_version property (and derived tag) to stay distinguishable after merging. Generated by Claude Code.
Uploading each sibling workflow's junit and Datadog coverage report as soon as that workflow finished meant firing datadog-ci/codecovcli child processes for every workflow at once whenever several finished in the same poll cycle, contending for the runner's CPU regardless of payload size. Neither junit tags nor Datadog's coverage flag vary per workflow (unlike Codecov's per-workflow flag, which still needs a call per run), so batch those two into one upload each, issued once every run is done, instead of one call per workflow. Generated by Claude Code.
A live rerun that forced several sibling workflows to finish at once downloaded 100+ artifacts in one uncapped burst; every fetch for one run failed with a generic "fetch failed" while the others succeeded, matching a connection/rate limit being hit under that burst size. Bound concurrency and retry transient failures, mirroring the existing upload retry pattern in run-upload.mjs. Generated by Claude Code
datadog-ci junit upload reads GITHUB_WORKFLOW/GITHUB_RUN_ID/GITHUB_RUN_NUMBER/ GITHUB_RUN_ATTEMPT from the process environment to attribute uploaded tests to a pipeline. Because all-green batched every sibling workflow's junit reports into one upload running from inside the all-green job, every test in Test Optimization showed up attributed to the "all-green" workflow instead of the one that actually produced it. Upload junit per sibling workflow run instead, tagged with that run's own metadata (already available from the GitHub API response all-green already fetches), alongside the existing per-run Codecov upload. Datadog coverage has no comparable per-test attribution to lose, so it stays batched into one call after every run finishes. Generated by Claude Code
datadog-ci junit upload reads GITHUB_WORKFLOW/GITHUB_RUN_ID/GITHUB_RUN_NUMBER from its own process env to set each test's Pipeline/Job facets. Uploading every sibling workflow's junit results from one batched call inside All Green attributed every test to the "All Green" workflow instead of the one that produced it. Revert the per-run-upload workaround from a previous commit (extra datadog-ci invocations, one per sibling workflow) in favor of stamping each job's own CI metadata as junit XML properties at mocha-run time (while its own GITHUB_* env vars are still correct), then lifting them into real per-test ci.pipeline.*/ci.job.name tags via --xpath-tag at upload time - mirroring the existing node_version tagging pattern, restoring the single batched upload. Also drop upload-junit.mjs's custom XML merging: --auto-discovery already recursively finds and uploads every matching file in one CLI invocation, so merging reports into one document first added regex-based XML surgery for no benefit. Generated by Claude Code.
…oads datadog-ci coverage upload recursively discovers every report file under a given path by default, so uploading each sibling workflow's already per-run-merged lcov file directly (one datadog-ci invocation over coverage-upload/) makes the extra merge into a single final lcov file redundant, mirroring the same simplification already applied to junit uploads. Generated by Claude Code.
…r-test pipeline tags getCISpanTags() derives ci.pipeline.name/id/number and ci.job.name straight from GITHUB_WORKFLOW/GITHUB_JOB/GITHUB_RUN_ID/GITHUB_RUN_NUMBER, applying the same value to every file in the batched junit upload -- overwriting the per-test values the --xpath-tag mappings lift from each testsuite's own stamped properties. Blanking just those four env vars for the datadog-ci child process leaves every other GITHUB_*-derived tag (repository URL, PR association, commit SHA) untouched, since none of those collide with what the xpath mappings set. Generated by Claude Code.
…d.mjs NodeJS.ProcessEnv isn't a recognized global type for this project's JSDoc lint setup; Record<string, string> matches the existing pattern used elsewhere in scripts/ for plain env-var-shaped objects. Generated by Claude Code.
Reruns of an already-fully-uploaded All Green job were re-running every sibling workflow's Codecov upload from scratch, saturating the runner with concurrent codecovcli processes. Persist the set of run IDs whose Codecov upload already succeeded (cached per-commit, updated after each run's own upload, not gated on the job finishing) so a rerun skips only the codecovcli call for runs already confirmed uploaded, while still redownloading/remerging their reports for the unconditional Datadog batch upload. Generated by Claude Code.
Skipping a cached run's codecovcli upload still paid for merging its istanbul JSON report, which only Codecov reads — istanbul-lib-coverage's merge is far slower than the lcov merge on a run with many cells, so it dominated rerun time even with the upload itself skipped. Pass skipJson through to mergeRunCoverage so a cached run only redoes the cheap lcov merge that the Datadog batch upload still needs. Generated by Claude Code.
istanbul JSON merging was the actual bottleneck on reruns (far slower than lcov's merge on runs with many cells), for coverage detail (branch/function) only Codecov read. Dropping JSON support outright removes that cost instead of caching around it, at the cost of losing that detail in Codecov. With the merge now cheap, the per-run Codecov-upload cache (a persisted run-id list plus an actions/cache step) no longer earns its complexity — every rerun just redoes download, merge, and upload for every run again. Generated by Claude Code.
…med lcov function merge Addresses two Codex/Datadog Autotest review findings on PR #9197: - The per-`downloadArtifacts`-call worker pool let each concurrently processed sibling workflow open its own 10-download burst, recreating the aggregate GitHub API pressure the cap was meant to prevent. A module-scoped Semaphore now bounds every in-flight call together. - `mergeLcovRecord` keyed functions by name only, so two functions sharing a name at different lines (e.g. two closures both named `shared`) collapsed into one, undercounting FNF/FNH. FN/FNDA are now paired positionally and keyed by `line,name`. Generated by Claude Code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd0a1681a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bbd0a16 to
2837855
Compare
Addresses Codex review feedback on bbd0a16: exporting Semaphore from download-artifacts.mjs existed only so its spec could reach in and instantiate it directly, expanding that module's API for test-only purposes. Moving it to scripts/semaphore.mjs makes it a real, independently testable utility instead. Also replaces the spec's real 5ms sleeps with deterministic microtask-flush assertions on promise resolution state, so the concurrency-cap tests no longer depend on wall-clock timing. Generated by Claude Code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0d7a8829e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ow status `checkAllGreen` only checked GitHub workflow-run conclusions before notifying Codecov, so a Codecov/coverage upload failure (which never throws, only sets `process.exitCode`) went unnoticed and the run still got treated as fully uploaded. Track upload failures in `run-upload.mjs` and gate on them too. Generated by Claude Code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba598e9d03
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…s OSes An artifact download that exhausts its retries only affected the log label, not the Codecov notification gate, so a transient GitHub outage on an otherwise-green run could still notify Codecov over a partial report. Track download failures the same way upload failures already are. Also normalize `SF:` paths before merging lcov records: istanbul's lcov writer uses `path.relative()`, which returns backslashes on a Windows matrix cell (e.g. AppSec's windows-2022 job) but forward slashes on Linux/macOS cells in the same run, so the same file's coverage was splitting into two separate records instead of summing. Generated by Claude Code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e7789799f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A thrown processRun error (e.g. the artifact listing or lcov merge itself failing) only set process.exitCode, so that run's report never reaching Codecov/Datadog went unnoticed by the notification gate. Generalize the existing incomplete-download flag to cover this path as well. Generated by Claude Code.
Summary
desired_execution_timefor the System Tests workflow from 300s (5 min) to 180s (3 min).ubuntujob into an[oldest, maintenance, active, latest]matrix so the four Node versions run in parallel instead of sequentially in one job.scripts/all-green.mjs,scripts/download-artifacts.mjs,scripts/upload-coverage.mjs).node_versiontag instead, and Datadog's coverage flag is the same for every run), so both are merged across every sibling workflow and uploaded once each, after all workflows finish, instead of once per workflow (scripts/upload-junit.mjs,scripts/group-coverage.mjs,scripts/upload-coverage.mjs). This was motivated by evidence that many CLI processes (datadog-ci/codecovcli) spawning at once when several sibling workflows finished in the same window were contending for CPU on the runner and inflating every upload's wall-clock time, regardless of payload size; reducing the number of invocations (rather than just capping concurrency, which only serializes the same work) fixes that at the source.node_versionproperty keeps cells distinguishable afterward (scripts/upload-junit.mjs).scripts/group-coverage.mjsmerges each workflow run's lcov reports into a single file, replacing the previous per-integration grouping (~100 groups uploaded per commit) with a real per-file merge (summingDA/FNDA/BRDAhit counts) rather than passing each cell's report through unmerged.istanbul-lib-coverage'smerge) turned out to be the actual bottleneck on reruns — far slower than the lcov merge on a run with many matrix cells — for coverage detail only Codecov's branch/function view used. Rather than caching around that cost, both uploads now go through the same merged lcov file and JSON support was removed outright (scripts/group-coverage.mjs,scripts/upload-coverage.mjs,.github/actions/upload-coverage-artifact/action.yml). The trade-off: Codecov reports line coverage only, no branch/function coverage, going forward — a conscious choice given how much rerun time the JSON merge cost for detail nothing else consumed.actions/cachestep) no longer earns its complexity — every rerun now just redoes download, merge, and upload for every run again (scripts/all-green.mjs,.github/workflows/all-green.yml).codecovcli do-uploadrequirescreate-commit/create-reportto already exist for the SHA;scripts/upload-coverage.mjsregisters the commit/report once per run (memoized, since every sibling workflow shares the same commit), and everycodecovclicall now passes--fail-on-errorso a real failure fails the job instead of being silently swallowed..codecov.ymlsetsnotify.manual_triggerso Codecov waits for a finalsend-notificationscall once every sibling workflow's uploads have landed, instead of posting its status after the first upload.scripts/run-upload.mjsretries eachcodecovcliupload call (2 retries, 2s backoff) before giving up, since--fail-on-errorcan now fail the whole job on a transient blip.scripts/download-artifacts.mjscaps how many artifacts it downloads concurrently and retries failed downloads with backoff, instead of firing every artifact's download at once — a run with 100+ artifacts finishing at once was observed hitting GitHub API connection/rate limits and failing every download for that run with a genericfetch failed.Test plan
./node_modules/.bin/mocha scripts/group-coverage.spec.mjs scripts/upload-junit.spec.mjspasses (updated for lcov-only merging)npx eslintclean on all changed filesstate: complete, upload sessions present, coverage computed)codecov/patchandcodecov/projectPR status checks post successfullyfetch failedfailures that an uncapped burst previously causedGenerated by Claude Code.